[Storage] Add sdk/storage/internal shared module - #27437
[Storage] Add sdk/storage/internal shared module#27437tanyasethi-msft (tanyasethi-msft) wants to merge 10 commits into
Conversation
Add the shared internal module for Azure Storage with: - structuredmsg package: XSM/1.0 encoder/decoder consolidated from azblob, azfile, and azdatalake - ValidateSeekableStreamAt0AndGetCount utility function - Module skeleton files (CHANGELOG.md, LICENSE.txt, README.md, go.mod)
|
Azure Pipelines: Successfully started running 1 pipeline(s). 6 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
/azp run prepare-pipelines |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
Adds a shared internal Storage module to consolidate structured-message functionality used by azblob, azfile, and azdatalake.
Changes:
- Adds XSM/1.0 streaming and in-memory encoders/decoders.
- Adds seekable-stream validation.
- Adds module metadata, documentation, and tests.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
sdk/storage/internal/version.go |
Defines the initial module version. |
sdk/storage/internal/util.go |
Adds seekable-stream validation. |
sdk/storage/internal/structuredmsg/structured_message.go |
Implements structured-message encoding and decoding. |
sdk/storage/internal/structuredmsg/structured_message_test.go |
Tests structured-message behavior and failures. |
sdk/storage/internal/README.md |
Documents the module. |
sdk/storage/internal/LICENSE.txt |
Adds MIT licensing. |
sdk/storage/internal/go.sum |
Records dependency checksums. |
sdk/storage/internal/go.mod |
Defines the Go module and dependencies. |
sdk/storage/internal/doc.go |
Documents the root package. |
sdk/storage/internal/CHANGELOG.md |
Records the initial release. |
Suppressed comments (2)
sdk/storage/internal/structuredmsg/structured_message.go:461
- Replace the corrupted replacement character in this comment.
// All segments done - emit trailer
sdk/storage/internal/structuredmsg/structured_message_test.go:1324
- The segment header is 10 bytes (
SMSegmentHeaderSize), so this byte-layout comment has the wrong size.
truncated := encoded[:truncateAt]
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Add uint16 segment cap to SMEncode (matching streaming encoder) - Convert io.EOF to io.ErrUnexpectedEOF on truncated segment reads - Remove msgLen > 0 guard so zero-length headers are always rejected - Assert io.ErrUnexpectedEOF in truncated decoder test - Fix incorrect segment header size in test comments (6 -> 10 bytes)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 1 comment.
Suppressed comments (7)
sdk/storage/internal/structuredmsg/structured_message.go:54
numSegmentscan exceed theuint16wire field (for example, 70,000 bytes withsegmentSize == 1). The casts in the header and segment numbers then wrap, so this function returns a malformed payload that cannot round-trip. The azfile and azdatalake copies already increasesegmentSizein this case; retain that protection in the consolidated implementation.
numSegments := totalDataLen / segmentSize
if totalDataLen%segmentSize != 0 {
numSegments++
sdk/storage/internal/structuredmsg/structured_message.go:312
- Converting
contentLentointbefore division corrupts lengths aboveMaxInton 32-bit builds. For example, a 3 GiB stream becomes a negative segment count, causing the encoder to emit only framing with a wrapped count instead of the content. Keep length and segment-count arithmetic inint64, converting only the final bounded segment count after enforcing theuint16limit.
// NewSMEncoder creates a streaming encoder that wraps the given content source.
// contentLen is the total size of the content (must be known upfront for the SM header).
// segmentSize specifies the max segment size; use 0 for the default (4MB).
sdk/storage/internal/structuredmsg/structured_message.go:806
- A zero message length bypasses validation entirely, so changing an otherwise valid response header's length field to zero is accepted by the streaming decoder. The one-shot decoder correctly rejects the same payload, and zero cannot be a valid encoded length because framing alone is nonempty. Compare unconditionally using unsigned values to preserve the full wire range.
}
}
sdk/storage/internal/structuredmsg/structured_message.go:820
- When the source returns the last framing bytes together with a non-EOF error, this branch discards that error and continues decoding. Segment-data reads explicitly propagate this case, but a transport error at a header/footer/trailer boundary can currently be reported as success. Preserve non-EOF errors even when the frame became full.
d.state = decStateDone
return nil
sdk/storage/internal/structuredmsg/structured_message_test.go:1198
- The segment header is 10 bytes (
SMSegmentHeaderSize), not 6 bytes. Update the boundary description so it matches the calculation below.
// Header (13 bytes) + segment header (10 bytes) + segment data (segSize bytes).
sdk/storage/internal/structuredmsg/structured_message_test.go:1301
- The segment header is 10 bytes, not 6 bytes; the comment currently contradicts
SMSegmentHeaderSizeused by the test.
// Truncate after segment data, partway through the segment footer.
sdk/storage/internal/structuredmsg/structured_message_test.go:1321
- The segment header is 10 bytes, not 6 bytes; update this size breakdown to match the actual XSM framing.
// Truncate after segment footer, partway through the message trailer.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 2 comments.
Suppressed comments (3)
sdk/storage/internal/structuredmsg/structured_message.go:206
- XSM/1.0 requires at least one segment, but the one-shot decoder accepts a crafted 21-byte message with
numSegments == 0and an empty CRC64 trailer. Reject zero here so malformed messages cannot bypass all segment validation.
numSegments := binary.LittleEndian.Uint16(smData[11:13])
sdk/storage/internal/structuredmsg/structured_message.go:816
- This length check only compares bytes consumed through the trailer with the embedded length. If a valid encoded message is followed by extra bytes while retaining its original
msgLen, the decoder reachesdecStateDonewithout reading those bytes and accepts the body, even though XSM requires message length to match the complete HTTP body. Validate the underlying body's end (or pass its known content length into the decoder) before reporting success.
// The consumed byte count must match the declared message length, so a stream that declares
// fewer segments (leaving trailing bytes unvalidated) is rejected rather than silently accepted.
if d.bytesRead != int64(d.msgLen) {
sdk/storage/internal/structuredmsg/structured_message.go:838
- The full-frame check runs before handling
err, so a source returning the final framing bytes together with a non-EOF error (for example, a transientnet.Error) has that error silently discarded. This defeats the retry behavior preserved in the segment-data path. Only suppress EOF when it accompanies a complete frame; propagate other errors first.
if d.frameHave >= d.frameNeed {
return true, nil
}
if err != nil {
… GetBlob Replace the GetProperties (HEAD) call in DownloadBuffer/DownloadFile with an initial GetBlob (GET) request to determine blob size. For small blobs (<=4MB), the entire content is returned in the initial response, eliminating an extra round trip. For larger blobs, the first chunk is consumed from the initial response and remaining chunks are downloaded in parallel. This reduces download latency by ~50% for blobs up to 4MB, which covers the vast majority of blob downloads (average blob size is 16KB).
Use uint32 loop counters in SMDecode and the streaming decoder to prevent infinite loops when numSegments is 65535 (MaxUint16).
… initial GetBlob" This reverts commit d2cde6e.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 14 changed files in this pull request and generated no new comments.
Suppressed comments (4)
sdk/storage/azblob/blob/client.go:518
- This newly added parallel path also leaves the retry reader and HTTP response body open when
io.Copyfails. Close it before returning so failed range downloads do not leak connections.
if _, err = io.Copy(shared.NewSectionWriter(writer, chunkStart+writerOffset, count), body); err != nil {
return err
}
sdk/storage/azblob/blob/client.go:393
- This writes the response ETag into the caller-owned
AccessConditions. Reusing the same options after the blob changes then sends the stale injectedIf-Matchand can fail unexpectedly; concurrent reuse can also race. Clone the nested condition structs before adding the per-download ETag.
} else if o.AccessConditions.ModifiedAccessConditions == nil {
o.AccessConditions.ModifiedAccessConditions = &ModifiedAccessConditions{IfMatch: dr.ETag}
} else if o.AccessConditions.ModifiedAccessConditions.IfMatch == nil {
o.AccessConditions.ModifiedAccessConditions.IfMatch = dr.ETag
sdk/storage/azblob/blob/client.go:592
- Skipping this block when
downloaded == 0leaves existing destination contents intact for an empty blob (or an empty range), despiteDownloadFilepromising to truncate the file to the downloaded size. Always compare and truncate, including to zero.
if downloaded > 0 {
sdk/storage/azblob/blob/client.go:474
- When copying a block fails, this returns without closing the retry reader and its active HTTP response body. With concurrent downloads, repeated read/write failures can leak connections and exhaust transport resources; close the body on this error path.
This issue also appears on line 516 of the same file.
if _, err = io.Copy(shared.NewSectionWriter(writer, chunkStart, count), body); err != nil {
return err
}
- Reject numSegments == 0 in the one-shot decoder since XSM requires at least one segment - Fix fillFrame to propagate non-EOF errors even when the frame buffer is full, instead of silently discarding them
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (2)
sdk/storage/internal/structuredmsg/structured_message.go:745
- The streaming decoder does not reject a zero segment count, unlike
SMDecode. If an otherwise valid one-segment message has bytes 11–12 cleared, this decoder still parses segment 1, treats it as the last segment (1 >= 0), validates the CRCs and length, and accepts malformed metadata. Reject zero here and add the streaming equivalent of the one-shot validation test.
d.numSegments = binary.LittleEndian.Uint16(buf[11:13])
sdk/storage/internal/structuredmsg/structured_message.go:875
- After a truncated or otherwise invalid header,
Readmoves the decoder todecStateError, so this condition no longer holds andDecodeResultreturns a non-nil, partially initialized result even though the documented prerequisite was never met. Track successful header parsing explicitly (for example, with a boolean set only afterparseHeadervalidates the header) and return nil until then; add a case using the existing truncated-body test.
if d.state == decStateHeader {
return nil
… ETag and data verification tests - Clone AccessConditions before mutating to avoid modifying caller's struct - Close body on io.Copy failure in parallelDownload and parallelDownloadFrom - DownloadFile now always truncates (including zero-byte blobs) - Fake transport returns deterministic bytes and ETag header - TestDownloadSmallBlobSkipsParallelRequests verifies actual buffer content - TestDownloadETagConsistency verifies If-Match header on follow-up requests
Keep the root internal package empty of functional code, grouping related helpers into sub-packages as Joel suggested.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 14 changed files in this pull request and generated 2 comments.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
sdk/storage/internal/structuredmsg/structured_message.go:745
- Unlike
SMDecode, the streaming decoder does not reject a zero segment count. If an otherwise valid one-segment message has bytes 11–12 cleared, this decoder still reads segment 1, treats1 >= 0as completion, validates both CRCs, and accepts the malformed message withNumSegments == 0. Reject zero here before entering the segment state.
d.numSegments = binary.LittleEndian.Uint16(buf[11:13])
| if o.AccessConditions != nil { | ||
| clone := *o.AccessConditions | ||
| ac = &clone | ||
| } | ||
| if ac.ModifiedAccessConditions == nil { | ||
| ac.ModifiedAccessConditions = &ModifiedAccessConditions{} | ||
| } | ||
| if ac.ModifiedAccessConditions.IfMatch == nil { | ||
| ac.ModifiedAccessConditions.IfMatch = dr.ETag | ||
| } |
| // It uses an initial GetBlob (GET) request instead of GetProperties (HEAD) to determine the blob size, | ||
| // eliminating an extra round trip for small blobs where the entire content is returned in the initial response. |
Summary
sdk/storage/internalshared module with structured message (XSM/1.0) encoder/decoder code consolidated from azblob, azfile, and azdatalakestructuredmsgsub-package andValidateSeekableStreamAt0AndGetCountutility